GH-45819: [C++] Add OptionalBitmapAnd utility function #45869
Conversation
github-actions
bot
added
Component: C++
awaiting committer review
|
@github-actions crossbow submit -g cpp |
|
Revision: 4188841 Submitted crossbow builds: ursacomputing/crossbow @ actions-fd8ca4245c
|
|
CI failures are unrelated. Happy to rebase once they are fixed. @pitrou is something like this what you had in mind? |
cpp/src/arrow/util/bitmap_ops.h
Outdated
| /// their respective bit-offsets for the given bit-length and put | ||
| /// the results in out_buffer starting at the given bit-offset. | ||
| /// Both right and left buffers are optional. If any of the buffers is | ||
| /// null a bitmap of zeros is returned. |
There was a problem hiding this comment.
Hmm, actually, the more useful semantics is that a null pointer means the bitmap is all-1s.
This reflects the situation where an Array doesn't have a null bitmap: all values are valid.
There was a problem hiding this comment.
And this would be even more useful as:
Result<std::shared_ptr<Buffer>> OptionalBitmapAnd(
MemoryPool* pool, const std::shared_ptr<Buffer>& left, int64_t left_offset,
const std::shared_ptr<Buffer>& right, int64_t right_offset, int64_t out_offset);... because then, if one of the inputs is null, and the offsets are compatible, we can return the other input (perhaps sliced) instead of allocating a new buffer.
|
@raulcd Would you like to revisit this? |
|
I re-started to try and adapt with the new APIs but I am finding adapting the existing tests for the new APIs slightly challenging, I will give that a try again but it is taking me quite a long time, this is the diff for my new branch: |
|
@pitrou this is currently working with the proposed API. I had to do several changes on the tests in order to adapt the existing |
|
|
||
| for (int64_t left_offset : {0, 1, 3, 5, 7, 8, 13, 21, 38, 75, 120, 65536}) { | ||
| BitmapFromVector(left_bits, left_offset, &left, &length); | ||
| if (left_bits.size() > 0) { |
There was a problem hiding this comment.
I think this is putting too much logic in these test methods. I would suggest something else:
- test
OptionalBitmapAndwith both non-null arguments like you do here - have separate tests for when one or both of the arguments is null
| if (right_offset == out_offset) { | ||
| return right; | ||
| } |
There was a problem hiding this comment.
We could easily slice the input buffer if both offsets are equal modulo 8. It would avoid copying in slightly more cases.
There was a problem hiding this comment.
It could be a dedicated function ViewOrCopyBitmap by the way. Search through the source code would probably find other places where such a function could be useful.
Rationale for this change
Bitmaps are optional, having the possibility an utility function that allows us to do a
BitmapAndoperation that takes into account the possibility of a bitmap being null can be handy as we don't have to cater for the specific case.What changes are included in this PR?
Add a new
OptionalBitmapAndfunction.Are these changes tested?
Yes.
Are there any user-facing changes?
No, just a new utility function.
OptionalBitmapAndutility #45819